std: sys: uefi: os: Implement join_paths#150993
std: sys: uefi: os: Implement join_paths#150993rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use |
723e209 to
e50d7bb
Compare
|
☔ The latest upstream changes (presumably #151107) made this pull request unmergeable. Please resolve the merge conflicts. |
e50d7bb to
988e7e2
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
988e7e2 to
0750de6
Compare
There was a problem hiding this comment.
LGTM.
Only thought I have is that this is basically the same implementation as the Windows join_paths, so they could share code. But I don't see a good existing place to put that shared code, and it's not a large amount of duplicated code, so I don't feel strongly about avoiding the duplication.
library/std/src/sys/pal/uefi/os.rs
Outdated
| if v.contains(&(b'"' as u16)) { | ||
| return Err(JoinPathsError); | ||
| } else if v.contains(&PATHS_SEP) { | ||
| joined.push(b'"' as u16); |
There was a problem hiding this comment.
Can you point to documentation suggesting it's valid to quote elements of the PATH? I'm not seeing anything to that effect in the linked specification.
There was a problem hiding this comment.
I tested on OVMF just now and indeed, while file names can contain ;, the path variable does not seem to support those paths. Using " does not seem to do anything (although it does not cause any error either).
So I have made ; an invalid character.
|
Reminder, once the PR becomes ready for a review, use |
0750de6 to
845f848
Compare
|
@rustbot ready |
845f848 to
85647ca
Compare
library/std/src/sys/pal/uefi/os.rs
Outdated
| joined.push(PATHS_SEP) | ||
| } | ||
| let v = path.encode_wide().collect::<Vec<u16>>(); | ||
| if v.contains("E) || v.contains(&PATHS_SEP) { |
There was a problem hiding this comment.
I don't think a path containing " is invalid (why do you think it is?) -- it just means that the file path has that character in it? The reason we need to forbid PATHS_SEP is that there's no way to escape that, so including it actually includes two paths (likely non-existent ones, though that's not guaranteed).
There was a problem hiding this comment.
I don't think a path containing
"is invalid (why do you think it is?) -- it just means that the file path has that character in it? The reason we need to forbid PATHS_SEP is that there's no way to escape that, so including it actually includes two paths (likely non-existent ones, though that's not guaranteed).
Well, according to UEFI shell spec, file name has the following rules: one or more ASCII characters, excluding * ? < > \ / ” : ). So from the perspective of path variable (which is specific to UEFI shell), a path cannot have ".
With that said, the reason of course is different.
;will give us an incorrect (i.e. something user might not have expected) path variable."will give us an invalid path.
So not sure if we need to throw error for " (and other invalid characters).
There was a problem hiding this comment.
My sense is that it doesn't make sense to attempt to validate that the components are valid paths in join_paths, we should only validate that the paths are accurately joined (i.e., each path on input is present in the output or an error is returned).
There was a problem hiding this comment.
Ok, I have removed the " check.
85647ca to
a7d25a7
Compare
library/std/src/sys/pal/uefi/os.rs
Outdated
| } | ||
|
|
||
| impl fmt::Display for JoinPathsError { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| "not supported on this platform yet".fmt(f) | ||
| "path segment contains `\"` or `;`".fmt(f) |
library/std/src/env.rs
Outdated
| @@ -518,8 +518,8 @@ pub struct JoinPathsError { | |||
| /// | |||
| /// Returns an [`Err`] (containing an error message) if one of the input | |||
| /// [`Path`]s contains an invalid character for constructing the `PATH` | |||
| /// variable (a double quote on Windows or a colon on Unix), or if the system | |||
| /// does not have a `PATH`-like variable (e.g. UEFI or WASI). | |||
| /// variable (a double quote on Windows or a colon on Unix and UEFI), or if | |||
There was a problem hiding this comment.
I think it's a semicolon on UEFI?
d7b193f to
3df2fa4
Compare
- PATHS_SEP is defined as global const since I will implement split_paths in the future. - Tested using OVMF using QEMU. Signed-off-by: Ayush Singh <ayush@beagleboard.org>
3df2fa4 to
90f0f4b
Compare
|
@bors r+ |
…imulacrum std: sys: uefi: os: Implement join_paths - Tested using OVMF using QEMU. @rustbot label +O-UEFI
…uwer Rollup of 10 pull requests Successful merges: - #150428 (UnixStream/UnixListener on Windows ) - #147400 (TryFrom<integer> for bool) - #150993 (std: sys: uefi: os: Implement join_paths) - #151483 (Add regression test for issue #138225) - #151568 (Update hexagon target linker configurations) - #151725 (Fix outdated Debian Ports ISO reference) - #151895 (Move UI tests batch) - #151923 (Fix ICE when projection error reporting sees opaque alias terms) - #151947 (Include assoc const projections in CFI trait object) - #151948 (Handle unbalanced delimiters gracefully in make_attr_token_stream)
Rollup of 11 pull requests Successful merges: - #151756 (Avoid miri error in `slice::sort` under Stacked Borrows) - #147400 (TryFrom<integer> for bool) - #150993 (std: sys: uefi: os: Implement join_paths) - #151483 (Add regression test for issue #138225) - #151568 (Update hexagon target linker configurations) - #151725 (Fix outdated Debian Ports ISO reference) - #151895 (Move UI tests batch) - #151923 (Fix ICE when projection error reporting sees opaque alias terms) - #151947 (Include assoc const projections in CFI trait object) - #151948 (Handle unbalanced delimiters gracefully in make_attr_token_stream) - #151963 (Add regression test for negative literal in a range of unsigned type)
Rollup of 11 pull requests Successful merges: - #151756 (Avoid miri error in `slice::sort` under Stacked Borrows) - #147400 (TryFrom<integer> for bool) - #150993 (std: sys: uefi: os: Implement join_paths) - #151483 (Add regression test for issue #138225) - #151568 (Update hexagon target linker configurations) - #151725 (Fix outdated Debian Ports ISO reference) - #151895 (Move UI tests batch) - #151923 (Fix ICE when projection error reporting sees opaque alias terms) - #151947 (Include assoc const projections in CFI trait object) - #151948 (Handle unbalanced delimiters gracefully in make_attr_token_stream) - #151963 (Add regression test for negative literal in a range of unsigned type)
@rustbot label +O-UEFI